Skip to content

clnrest: add more request and response types #8383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 25, 2025
Merged

clnrest: add more request and response types #8383

merged 1 commit into from
Jul 25, 2025

Conversation

daywalker90
Copy link
Collaborator

@daywalker90 daywalker90 commented Jun 27, 2025 •
edited
Loading

Changelog-Added: clnrest can now return successful responses as xml, yaml, html or form-encoded in addition to json defined in the 'Accept' header. The same goes for request types except for html defined in the 'Content-type' header.

Important

25.09 FREEZE July 28TH: Non-bugfix PRs not ready by this date will wait for 25.12.

RC1 is scheduled on August 11th

The final release is scheduled for September 1st.

Fixes #7164

Right now the html response type is just the json wrapped as text in an html body and for requests i did not add html.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@daywalker90 daywalker90 mentioned this pull request Jun 27, 2025
Closed
@daywalker90 daywalker90 force-pushed the clnrest-return-types branch from 36b5d63 to 3c27f6d Compare June 27, 2025 17:41
@ShahanaFarooqui
Copy link
Collaborator

BTW, the PR is clearly WIP but thought to post my test cases here for future review. Feel free to use them in test suite if they seem helpful.

-----------------------
Request:
curl -k -X 'POST' 'https://127.0.0.1:3010/v1/getinfo' -H 'rune: MyRune' -d '{}'

Response:
{"code":null,"data":null,"message":"Unsupported accept header: */*"}

Expectation:
Should respond getinfo response in json (default to application/json).

-----------------------
Request:
curl -k -X 'POST' 'https://127.0.0.1:3010/v1/getinfo' -H 'rune: MyRune' -d '{}' -H 'accept: application/json' -H 'Content-Type: application/json'

Response:
Successful getinfo json response.

Expectation:
Successful getinfo json response.

-----------------------
Request:
curl -k -X 'POST' 'https://127.0.0.1:3010/v1/newaddr' -H 'rune: MyRune' -d '{"addresstype": "p2tr"}' -H 'accept: application/json' -H 'Content-Type: application/json'

Response:
Successful json response with p2tr address.

Expectation:
Successful json response with p2tr address.


-----------------------
Request:
curl -k -X 'POST' 'https://127.0.0.1:3010/v1/newaddr' -H 'rune: MyRune' -d '{"addresstype": "p2tr"}' -H 'accept: application/xml' -H 'Content-Type: application/json'

Response:
Successful xml response with p2tr address.

Expectation:
Successful xml response with p2tr address.


-----------------------
Request:
curl -k -X 'POST' 'https://127.0.0.1:3010/v1/newaddr' -H 'rune: MyRune' -d '<addresstype>p2tr</addresstype>' -H 'accept: application/xml' -H 'Content-Type: application/xml'

Response:
Responded with bech32 address in xml format.

Expectation:
It should respond with p2tr address in xml format but `Content-Type: application/xml` doesn't work and the request generates default bech32 addres instead.

@daywalker90 daywalker90 force-pushed the clnrest-return-types branch from 3c27f6d to 733601e Compare June 28, 2025 19:30
@daywalker90 daywalker90 changed the title clnrest: add xml, yaml, html and form-encoded response types clnrest: add more request and response types Jun 28, 2025
@daywalker90
Copy link
Collaborator Author

I've added support for different request types (except html, how would that work?). I also added some tests and improved the documentation/compatibility with the swagger ui.

@daywalker90
Copy link
Collaborator Author

haha what is the CI smoking:

newaddr = l1.rpc.newaddr(addresstype='p2tr')['p2tr']

pyln.client.lightning.RpcError: RPC call failed: method: newaddr, payload: {'addresstype': 'p2tr'}, error: {'code': -32602, 'message': "'addresstype' should be 'p2tr', 'bech32', or 'all', not 'p2tr'"}

@daywalker90 daywalker90 force-pushed the clnrest-return-types branch from 733601e to 277730d Compare June 28, 2025 21:32
@ShahanaFarooqui
Copy link
Collaborator

I've added support for different request types (except html, how would that work?). I also added some tests and improved the documentation/compatibility with the swagger ui.

For now, we can proceed without adding HTML type support and will reconsider in future if a compelling use case arises.

@daywalker90 daywalker90 force-pushed the clnrest-return-types branch from 277730d to 7e44466 Compare June 30, 2025 13:32
@daywalker90
Copy link
Collaborator Author 

pyln.client.lightning.RpcError: RPC call failed: method: newaddr, payload: {'addresstype': 'p2tr'}, error: {'code': -32602, 'message': "'addresstype' should be 'p2tr', 'bech32', or 'all', not 'p2tr'"}

Apparently this isn't a weird CI bug but liquid just doesn't allow p2tr addresses (Is this known?). I've added a skip on liquid for those tests.

For now, we can proceed without adding HTML type support and will reconsider in future if a compelling use case arises.

Yes, i removed the html type from responses.

@ShahanaFarooqui ShahanaFarooqui self-requested a review July 24, 2025 14:15
@ShahanaFarooqui ShahanaFarooqui added this to the v25.09 milestone Jul 24, 2025
@daywalker90 daywalker90 force-pushed the clnrest-return-types branch from 7e44466 to 5d639ee Compare July 24, 2025 14:38
@daywalker90
Copy link
Collaborator Author

rebased and got a clean ci run. iirc previous ci errors were unrelated.

@ShahanaFarooqui ShahanaFarooqui force-pushed the clnrest-return-types branch 2 times, most recently from 9e64f68 to 8b6b2c1 Compare July 25, 2025 06:45
ShahanaFarooqui
ShahanaFarooqui approved these changes Jul 25, 2025
View reviewed changes
Copy link
Collaborator

@ShahanaFarooqui ShahanaFarooqui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 8b6b2c1.

@daywalker90 @ShahanaFarooqui
clnrest: add more valid request and response types
b5c4107 
Changelog-Added: clnrest can now return successful responses as xml, yaml, or form-encoded in addition to json defined in the 'Accept' header. The same goes for request types defined in the 'Content-type' header.
@ShahanaFarooqui ShahanaFarooqui merged commit bf37d41 into ElementsProject:master Jul 25, 2025
76 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShahanaFarooqui ShahanaFarooqui ShahanaFarooqui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v25.09
Development

Successfully merging this pull request may close these issues.

clnrest: dynamic content-type and accept headers
2 participants
@daywalker90 @ShahanaFarooqui